fix: Stop series data not saving when max values reached [PT-187949831] #167
Conversation
Previously there was a useCallback() wrapper on the stop time series function that depended on the formData but this caused issues with the formData being captured as an empty array at the start of multiple runs where the runs hit their max number of values. This fix adds a ref to the formData so that the closure is not created.
There was a problem hiding this comment.
Well, I somehow can't connect this explanation with the code changes. The previous useCallback() looked perfectly fine to me. The onSensorStopTimeSeries handler was recreated with each formData update, as it probably should be.
I could imagine that some code was calling setFormData incorrectly, most likely without creating a new object, and hence your callback was not updated. I think this could be a great opportunity to track it down, actually.
The current approach looks a bit brutal. I'm a bit worried we might be dealing with the issue at a deeper level, and it might surface later, making it harder to debug.
I feel that simply removing useCallback, even without using a ref, could have a similar result to what we currently have, wouldn't it? Although that still probably doesn't solve the core issue.
But I might not understand it well enough.
|
This function: closed over the current value of I've tested this change and it works for both the manual stop button (which worked before) and the automatic stop (which was not working). |
|
Ah, I see it better now. However, I think we're fixing it on the wrong side, and there are definitely more issues in this code. If we add anything else (e.g., a new state) to the I think the last argument of
Currently, it references Probably both fixes should be combined, and it might look like this: stopTimeSeriesFnRef.current = sensor.collectTimeSeries(timeSeriesCapabilities.measurementPeriod, selectableSensorId, useCallback((values) => {
if (values.length <= MaxNumberOfTimeSeriesValues) {
setFormData(prevData => {
const newData = prevData.slice();
newData[rowIdx] = {...newData[rowIdx], timeSeries: values, timeSeriesMetadata};
return newData;
});
}
if (values.length === MaxNumberOfTimeSeriesValues) {
onSensorStopTimeSeries();
}
}, [rowIdx, onSensorStopTimeSeries, /* probably more things here */]);
timeSeriesRecordingRowRef.current = rowIdx;
};And |
|
@pjanik I tried your code and the callback passed to |
|
Yeap, I haven't tested it, but I'd think that's the correct direction. But sure, let's merge it as it is. |
Previously there was a useCallback() wrapper on the stop time series function that depended on the formData but this caused issues with the formData being captured as an empty array at the start of multiple runs where the runs hit their max number of values.
This fix adds a ref to the formData so that the closure is not created.